Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

frontend: Add feature to view deploy logs #2581

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

knrt10
Copy link
Contributor

@knrt10 knrt10 commented Nov 18, 2024

This adds feature to view all the pods of pods in a deployment rather than going to a specific pod.

Fixes: #2552

Testing

  • Create a deployment kubectl create deployment nginx --image=nginx --replicas=3
  • Run the app and navigate to the deployment
  • Click on the logs button to view the logs of all the pods in the deployment

Screenshots

Screenshot 2024-11-29 at 2 59 43 PM Screenshot 2024-11-29 at 2 59 50 PM

@knrt10 knrt10 requested a review from a team November 18, 2024 02:59
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Nov 18, 2024
@knrt10
Copy link
Contributor Author

knrt10 commented Nov 18, 2024

So weird, I ran the tests locally, updated snapshots, wonder why it is still failing 🤔

@knrt10 knrt10 marked this pull request as draft November 18, 2024 09:50
@knrt10
Copy link
Contributor Author

knrt10 commented Nov 18, 2024

Changing some UX for this

@knrt10 knrt10 marked this pull request as ready for review November 29, 2024 09:31
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Nov 29, 2024
@knrt10
Copy link
Contributor Author

knrt10 commented Nov 29, 2024

Updated, please review

@knrt10 knrt10 force-pushed the multi-pods-logs branch 3 times, most recently from 46a12fb to 7daec5f Compare December 2, 2024 03:00
@knrt10 knrt10 force-pushed the multi-pods-logs branch 2 times, most recently from 7848c4f to 016f070 Compare December 16, 2024 09:21
@illume illume requested a review from Copilot January 3, 2025 07:58

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 41 out of 57 changed files in this pull request and generated no comments.

Files not reviewed (16)
  • frontend/src/components/common/Resource/MainInfoSection/snapshots/MainInfoSection.Normal.stories.storyshot: Language not supported
  • frontend/src/components/common/Resource/MainInfoSection/snapshots/MainInfoSection.NullBacklink.stories.storyshot: Language not supported
  • frontend/src/components/configmap/snapshots/Details.Empty.stories.storyshot: Language not supported
  • frontend/src/components/configmap/snapshots/Details.WithBase.stories.storyshot: Language not supported
  • frontend/src/components/crd/snapshots/CustomResourceDefinition.Details.stories.storyshot: Language not supported
  • frontend/src/components/crd/snapshots/CustomResourceDetails.NoError.stories.storyshot: Language not supported
  • frontend/src/components/cronjob/snapshots/CronJobDetails.EveryAst.stories.storyshot: Language not supported
  • frontend/src/components/cronjob/snapshots/CronJobDetails.EveryMinute.stories.storyshot: Language not supported
  • frontend/src/components/endpoints/snapshots/EndpointDetails.Default.stories.storyshot: Language not supported
  • frontend/src/components/endpoints/snapshots/EndpointDetails.Error.stories.storyshot: Language not supported
  • frontend/src/components/horizontalPodAutoscaler/snapshots/HPADetails.Default.stories.storyshot: Language not supported
  • frontend/src/components/horizontalPodAutoscaler/snapshots/HPADetails.Error.stories.storyshot: Language not supported
  • frontend/src/components/ingress/snapshots/ClassDetails.Basic.stories.storyshot: Language not supported
  • frontend/src/components/ingress/snapshots/ClassDetails.WithDefault.stories.storyshot: Language not supported
  • frontend/src/components/ingress/snapshots/Details.WithResource.stories.storyshot: Language not supported
  • frontend/src/components/ingress/snapshots/Details.WithTLS.stories.storyshot: Language not supported
This adds feature to view all the pods of pods in a deployment rather
than going to a specific pod.

Fixes: #2552

Signed-off-by: Kautilya Tripathi <[email protected]>
Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be a very useful feature for people. It's very often the case I've personally wanted to look at all the logs in a deployment rather than ones from just one pod.

I think the error states could be added for if any request fails. Left a couple of notes in the spots I think they could be handled.

Maybe not in this PR... but it would probably good to add a loading state? I'm not sure if the logs already have a loading state? (I didn't look).

Adding some states into a LogsButton.stories.tsx would be nice to have.

// Handler for initial logs button click
async function handleClick() {
if (item instanceof Deployment) {
const fetchedPods = await getRelatedPods();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be error handling for this. With a try/catch, and telling the user that something went wrong if something does.

async function getRelatedPods(): Promise<Pod[]> {
if (item instanceof Deployment) {
const labelSelector = item.getMatchLabelsList().join(',');
const response = await request(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be error handling for this. With a try/catch, and telling the user that something went wrong if something does.

{/* Follow logs switch */}
<FormControlLabel
control={<Switch checked={follow} onChange={handleFollowChange} size="small" />}
label="Follow"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Label should be translated.

{/* Timestamps switch */}
<FormControlLabel
control={<Switch checked={showTimestamps} onChange={handleTimestampsChange} size="small" />}
label="Timestamps"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Label should be translated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FEATURE REQUEST - Multi pod logs
2 participants